Although the getAccounts's return type is (InternalAccount | undefined)[], we're sure to get back all the accounts we want since the accounts list will never be stale.

Decided to get rid of this mapping since it was only being used for handling the accountRemoved and accountAdded events, removing this gets rid of a large loop in the init function as well. If there's a particular need for this data at the client level, we can always add this back in.

See my other comment about:

Given the providers are the sources of accounts...

Given the providers are the sources of accounts, I don't think they should have an addAccounts method? 🤔

They should be able to hold a list of known accounts once they are initialized? And since we're sharing the same providers instances across the service/wallets/groups, this initialization should happen only once. Further updates to the internal account list happens when a createAccounts is called on that providers.

WDYT?

This is to more so maintain a global list of the ids and reduce the number of calls/work we would have to make to other controllers. I wanted it this way because we were just grabbing the entire list of accounts from the AccountsController every time we called the getAccount(s) in the group and provider classes, so maintaining a global list that the provider can fetch from the new AccountsController:getAccounts method made sense to me.

Well, I think that makes sense that providers keep an internal list of account IDs yes. But the consumers (wallet/group/service here) should have no control over that IMO.

Like, when we call createAccounts we know we have to update this internal list with new accounts. Same goes for removeAccounts, if a provider is getting disabled, then it should automatically update its internal list IMO, it's not the consumer role to make sure this list is "consistent".

However, consumers can still react to those actions/events and update their own mapping (basically the #accountToProviders maps here).

That's why I wanted to avoid to have some state in the providers initially, mainly to not have those mutable methods. But I still think that your internal state makes sense and align with the existing interface, so we should be able to make this work still, no?

Hmm that's a fair assessment, I suppose I can move the addAccounts call to createAccounts and have the AccountProviderWrapper clear it's internal list if the provider is disabled when createAccounts is called in alignment?

I wonder if we could omit the init part?

I really like that idea of "passing" a in-memory-state to the wallet/groups, because indeed, they are just "wrapper" around methods for their own domain (wallet domain or group domain).

Having the service being the owner of the entire memory layout and passing "views" on each "wallet/group state" would make sense to me.

And I think we could have a similar pattern on the providers too, so the providers can update the state directly, and since every components would share the same "data views", they would get updated automatically too.

Just need to double-check for concurrent accesses if we start sharing the same spaces though 😅

Hmm I'd prefer to keep this pattern, it feels less complicated and more explicit about the intended action to the wallet/group state. I think we would have to end up changing the provider interface as well and would essentially be moving computation here to the provider. We're still reacting to the provider's actions by initing with additional state at the points of creation.

Alright, then WDYT about adding it in the constructor's params? So we are "forced" to pass the initial state of the wallet?

This way, we can also rename init to setState maybe, and re-use it in other contexts (cause I think init makes sense when we init right after constructing an object, but it does sounds "bad" when we just re-use it to "reset" the internal state, so reset or setState could be more appropriate for those).

WDYT?

Yeah, I can name it something else.

Ok now I remember why I was against the idea of passing it through the constructor, not every wallet will have an initial state i.e. new wallets that haven't had any discovery ran on it. I will however add another method and call it updateState instead of calling init (it'll be just the init call under the hood anyway, but for clarity purpose we can call it something else).

Maybe use multiple accounts instead?

IMO we should re-introduce the generic parameter for those providers. It was meant to be "extensible" and would align it with the Account type parameter being used by the top-level class here

There was a good reason I had to change this, the types were not compatible for some reason which I'm forgetting why honestly (it's been a while this PR has been open 😅). I'll go back and change and see if it's compatible if not, I'll report back here.

We need the cast because the providers are not using the Account type parameter. This cast should be gone if we re-add it I guess

See above.

I would not use an error message in that case. Maybe we can re-use what you did in the EvmAccountProvider.#createAccount last time, like returning a boolean to indicate if we created accounts or not?

Like const [didCreate, accounts] = .... WDYT?

Do you mean changing the provider's createAccounts return type or returning the tuple in the promise in Promise.allSettled? I found this method easier because then we're returning a tuple just for this one use case.

I start to think we should introduce a .id for providers too 🤔 Name is sort of unique today, but I feel like it's more appropriate for logging/debugging.

On the other hand id fits more in our patterns, and given that we now use it a lot to "map" things, it sounds better to me. WDYT?

I don't know if it's worth the effort to do that but I'm not completely opposed to it, a provider's name would be unique otherwise it wouldn't be serving it's purpose haha. Are you proposing assigning some uuid in the provider's constructor?

Alright, then WDYT about adding it in the constructor's params? So we are "forced" to pass the initial state of the wallet?

This way, we can also rename init to setState maybe, and re-use it in other contexts (cause I think init makes sense when we init right after constructing an object, but it does sounds "bad" when we just re-use it to "reset" the internal state, so reset or setState could be more appropriate for those).

WDYT?

We should probably also split this from this PR. For similar reasons (see my other comment about packages/keyring-controller/src/KeyringController.ts file)

We also need that cast because our provider type does not use the generic type of this class. So IMO, we should re-introduce it.

See above.

Make sure this method is fast enough, it's called very often and previously it was quite slow. You can do something like this to check it:

getAccounts(): Bip44Account<KeyringAccount>[] {
     const perf = performance.now()
     // ... method code
     console.log("getAccounts perf", performance.now() - perf)
    return internalAccounts
}

Yeah, previously this method would iterate through the whole list of accounts but now I've added a method to the accounts controller to just do constant time lookup for each account id. With the refactor we also aren't relying on this method anymore for state construction either so this will no longer be an issue.

Bug: Service State Initialization Fails for Non-HD Keyrings

The #constructServiceState method initializes serviceState only for HD keyrings. If an account's entropy source doesn't correspond to an HD keyring, subsequent attempts to access serviceState[entropySource][groupIndex] will result in a "Cannot read property 'groupIndex' of undefined" error.